OPRUN-4070: [OTE] add webhook tests #424
Merged
+464
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
camilamacedo86
force-pushed
the
webhook-tests
branch
6 times, most recently
from
fa9edc8 to
80b830a
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
webhook-tests
branch
2 times, most recently
from
aff5c1a to
515db30
Compare
openshift-merge-robot
added
the
needs-rebase
camilamacedo86
force-pushed
the
webhook-tests
branch
from
515db30 to
533bccb
Compare
openshift-merge-robot
removed
the
needs-rebase
camilamacedo86
force-pushed
the
webhook-tests
branch
5 times, most recently
from
1452c6e to
6530e38
Compare
camilamacedo86
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@camilamacedo86: This pull request references OPRUN-4070 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
webhook-tests
branch
2 times, most recently
from
9104ae3 to
231f5cc
Compare
Migrates OLMv1 webhook operator tests from using external YAML files to defining resources in Go structs. This change removes file dependencies, improving test reliability and simplifying test setup. The migration is a refactoring of code from openshift/origin#30059. The new code uses better naming conventions and adapts the tests to work with a controller-runtime client, enhancing test consistency and maintainability. The migration covers all core test scenarios: - Validating, mutating, and conversion webhooks. - Certificate and secret rotation tolerance. Assisted-by: Gemini
camilamacedo86
force-pushed
the
webhook-tests
branch
from
231f5cc to
19447d0
Compare
|
/test openshift-e2e-aws |
|
@camilamacedo86: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
anik120
suggested changes
Aug 12, 2025
There was a problem hiding this comment.
The commit message description is confusing to me...how about something like this:
UPSTREAM: <carry>: [OTE] add webhook tests
This commit adds webhooks test to OTE, by lifting the tests from https://github.com/openshift/origin/pull/30059
Additional work done after lifting the tests from https://github.com/openshift/origin/pull/30059:
1. Switched from using external YAML files to
defining resources in Go structs. This change removes file dependencies,
improving test reliability and simplifying test setup.
2. Switched to using a dynamic client (instead of `oc` used in origin tests).
Assisted-by: Gemini
Comment on lines
+41
to
+60
| var ( | ||
| k8sClient client.Client | ||
| dynamicClient dynamic.Interface | ||
| webhookOperatorInstallNamespace string | ||
| cleanup func(ctx context.Context) | ||
| ) | ||
|
|
||
| BeforeEach(func(ctx SpecContext) { | ||
| By("initializing Kubernetes client and dynamic client") | ||
| k8sClient = env.Get().K8sClient | ||
| restCfg := env.Get().RestCfg | ||
| var err error | ||
| dynamicClient, err = dynamic.NewForConfig(restCfg) | ||
| Expect(err).ToNot(HaveOccurred(), "failed to create dynamic client") | ||
|
|
||
| By("requiring OLMv1 capability on OpenShift") | ||
| helpers.RequireOLMv1CapabilityOnOpenshift() | ||
|
|
||
| By("ensuring no ClusterExtension and CRD from a previous run") | ||
| helpers.EnsureCleanupClusterExtension(ctx, webhookOperatorPackageName, webhookOperatorCRDName) |
There was a problem hiding this comment.
Suggested change
| var ( | |
| k8sClient client.Client | |
| dynamicClient dynamic.Interface | |
| webhookOperatorInstallNamespace string | |
| cleanup func(ctx context.Context) | |
| ) | |
| BeforeEach(func(ctx SpecContext) { | |
| By("initializing Kubernetes client and dynamic client") | |
| k8sClient = env.Get().K8sClient | |
| restCfg := env.Get().RestCfg | |
| var err error | |
| dynamicClient, err = dynamic.NewForConfig(restCfg) | |
| Expect(err).ToNot(HaveOccurred(), "failed to create dynamic client") | |
| By("requiring OLMv1 capability on OpenShift") | |
| helpers.RequireOLMv1CapabilityOnOpenshift() | |
| By("ensuring no ClusterExtension and CRD from a previous run") | |
| helpers.EnsureCleanupClusterExtension(ctx, webhookOperatorPackageName, webhookOperatorCRDName) | |
| var ( | |
| webhookOperatorInstallNamespace string | |
| cleanup func(ctx context.Context) | |
| ) | |
| k8sClient := env.Get().K8sClient | |
| dynamicClient, err := dynamic.NewForConfig(env.Get().RestCfg) | |
| Expect(err).ToNot(HaveOccurred(), "failed to create dynamic client") | |
| helpers.RequireOLMv1CapabilityOnOpenshift() | |
| BeforeEach(func(ctx SpecContext) { | |
| By("ensuring no ClusterExtension and CRD from a previous run") | |
| helpers.EnsureCleanupClusterExtension(ctx, webhookOperatorPackageName, webhookOperatorCRDName) | |
Why do this in BeforeEach block? This can be done once
There was a problem hiding this comment.
I agree that we could do as suggested but that does not change the result/check is doing the same so I think is fine get done in a follow up
There was a problem hiding this comment.
I'm not a fan of the global variables either. But the net result is the same.
There was a problem hiding this comment.
Acctually we cannot do the suggestion with OTE lib
Ginkgo detected an issue with your spec structure
By("initializing Kubernetes client")
/Users/camilam/go/src/github/operator-framework-operator-controller/openshift/tests-extension/test/webhooks.go:45
It looks like you are calling By outside of a running spec. Make sure you
call By inside a runnable node such as It or BeforeEach and not inside the
body of a container such as Describe or Context.
Learn more at: http://onsi.github.io/ginkgo/#documenting-complex-specs-by
make: *** [update-metadata] Error 1
It will fail like above ^
Comment on lines
+80
to
+85
| AfterEach(func(ctx SpecContext) { | ||
| By("performing webhook operator cleanup") | ||
| if cleanup != nil { | ||
| cleanup(ctx) | ||
| } | ||
| }) |
There was a problem hiding this comment.
nit: this is missing a pod log dump that could be helpful in debugging when things go wrong https://github.com/openshift/origin/pull/30059/files#diff-6dd6fa78ac85235012d3c9910f8510bc1640c830a83888681bd5922cec0dffcbR50-R57
There was a problem hiding this comment.
We did not add it yet. We can add in a follow-up.
We can track it as a tech debt, and we can add it to all tests afterwards.
There was a problem hiding this comment.
Just to clarify, we need a helper/utils with and that could be used in all tests
There was a problem hiding this comment.
HI @anik120 we will add that in a follow up but look that the original code is wrong
The clean ( which will remove the pods ) is called before the dump, so that would not work.
Comment on lines
+100
to
+103
| // Webhook not ready yet; clean up and keep polling. | ||
| _ = dynamicClient.Resource(webhookTestGVRV1). | ||
| Namespace(webhookOperatorInstallNamespace). | ||
| Delete(ctx, name, metav1.DeleteOptions{}) |
There was a problem hiding this comment.
why is there a deletion of the web hook happening here?
This is within an eventually block..isn't it sufficient to
- Check if there is an error
- Check if the error contains the substring we expect
The switch case block is just adding verbosity
There was a problem hiding this comment.
The switch is needed to ensure consistent behavior.
Before the webhook server is fully up and returning the expected rejection, we still need to wait until we get that specific error.
There are many ways to implement this — and we might find a better approach later — but since this meets our needs now, I suggest we move forward and handle any improvements in a follow-up.
Comment on lines
+241
to
+251
| var webhookTestGVRV1 = schema.GroupVersionResource{ | ||
| Group: "webhook.operators.coreos.io", | ||
| Version: "v1", | ||
| Resource: "webhooktests", | ||
| } | ||
|
|
||
| var webhookTestGVRV2 = schema.GroupVersionResource{ | ||
| Group: "webhook.operators.coreos.io", | ||
| Version: "v2", | ||
| Resource: "webhooktests", | ||
| } |
There was a problem hiding this comment.
Suggested change
| var webhookTestGVRV1 = schema.GroupVersionResource{ | |
| Group: "webhook.operators.coreos.io", | |
| Version: "v1", | |
| Resource: "webhooktests", | |
| } | |
| var webhookTestGVRV2 = schema.GroupVersionResource{ | |
| Group: "webhook.operators.coreos.io", | |
| Version: "v2", | |
| Resource: "webhooktests", | |
| } | |
| var webhookGVRv1 = schema.GroupVersionResource{ | |
| Group: "webhook.operators.coreos.io", | |
| Version: "v1", | |
| Resource: "testwebhook", | |
| } | |
| var webhookGVRv2 = schema.GroupVersionResource{ | |
| Group: "webhook.operators.coreos.io", | |
| Version: "v2", | |
| Resource: "testwebhook", | |
| } |
There was a problem hiding this comment.
Matter of preference: we can change in a follow up
There was a problem hiding this comment.
I'm not that concerned about variable names... there's a lot here that is basically a nit.
There was a problem hiding this comment.
We cannot change the names see the orginal it uses webhooktests
v1WebhookTestClient := c.Resource(schema.GroupVersionResource{
Group: "webhook.operators.coreos.io",
Version: "v1",
Resource: "webhooktests",
}).Namespace(webhookOperatorInstallNamespace)
That needs to match with the solution
There was a problem hiding this comment.
We cannot do that. It will fail does not match with the orginal as well
Please, see the original PR. The kind must match with what is in the sample.
| Resource: "webhooktests", | ||
| } | ||
|
|
||
| func newWebhookTestV1(name, namespace string, valid bool) *unstructured.Unstructured { |
There was a problem hiding this comment.
Suggested change
| func newWebhookTestV1(name, namespace string, valid bool) *unstructured.Unstructured { | |
| func newTestWebhook(name, namespace string, valid bool) *unstructured.Unstructured { |
There was a problem hiding this comment.
Matter of preference: I am oK with but we can do in a follow up
| obj := &unstructured.Unstructured{ | ||
| Object: map[string]interface{}{ | ||
| "apiVersion": "webhook.operators.coreos.io/v1", | ||
| "kind": "WebhookTest", |
There was a problem hiding this comment.
Suggested change
| "kind": "WebhookTest", | |
| "kind": "TestWebhook", |
There was a problem hiding this comment.
Matter of preference: I am oK with but we can do in a follow up
There was a problem hiding this comment.
We cannot do that fails does not match wiht solution see original https://github.com/openshift/origin/pull/30059/files#diff-6dd6fa78ac85235012d3c9910f8510bc1640c830a83888681bd5922cec0dffcbR334
There was a problem hiding this comment.
We cannot do that. It will fail does not match with the orginal as well
Please, see the original PR. The kind must match with what is in the sample.
| }) | ||
|
|
||
| It("should have a working conversion webhook", func(ctx SpecContext) { | ||
| By("creating a conversion webhook test resource") |
There was a problem hiding this comment.
Suggested change
| By("creating a conversion webhook test resource") | |
| By("creating a conversion webhook") |
There was a problem hiding this comment.
Just a log: Nit I think is fine change in a follow up
|
Hi @anik120, Everything you added here doesn’t seem to be a blocker for merging. They’re mostly variable renames, log improvements, and commit message changes. Based on the review, I understand that we are here doing the same tests, am I right? If we change the PR now, we’ll need to rerun all the tests. Since none of the points raised are actual blockers or compromise the primary goal, I’d suggest we merge them as is, monitor the tests in Sippy, and then handle those improvements in a follow-up. Does that make sense? It would make sense not to close the webhook tests based on these improvements, or can we for sure do them in a follow-up? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
that is true...I don't see any blocking issue in this PR. Let's get it in and start observing sippy /lgtm |
openshift-merge-bot
bot
merged commit 9963614
into
openshift:main
12 of 13 checks passed
|
Hi @anik120 But I will work in a follow up to try address all your comments |
|
[ART PR BUILD NOTIFIER] Distgit: ose-olm-operator-controller |
|
[ART PR BUILD NOTIFIER] Distgit: ose-olm-catalogd |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Migrates OLMv1 webhook operator tests from using external YAML files to defining resources in Go structs. This change removes file dependencies, improving test reliability and simplifying test setup.
The migration is a refactoring of code from openshift/origin#30059. The new code uses better naming conventions and adapts the tests to work with a controller-runtime client, enhancing test consistency and maintainability.
The migration covers all core test scenarios:
Local Test
Assisted-by: Gemini